fix(mcp): The check workspace tool should not fail when missing organization permissions#905
Conversation
…workspace Co-Authored-By: AJ Steers <aj@airbyte.io>
Original prompt from AJ Steers |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This PyAirbyte VersionYou can test this version of PyAirbyte using the following: # Run PyAirbyte CLI from this branch:
uvx --from 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1765397982-fix-check-workspace-permissions' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1765397982-fix-check-workspace-permissions'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
📝 WalkthroughWalkthroughcheck_airbyte_cloud_workspace now retrieves organization info via CloudWorkspace.get_organization(raise_on_error=False) and constructs CloudWorkspaceResult using the optional CloudOrganization. If organization data is inaccessible (permission error), organization_id becomes "[unavailable - requires ORGANIZATION_READER permission]" and organization_name becomes None. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/mcp/cloud_ops.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: yohannj
Repo: airbytehq/PyAirbyte PR: 716
File: airbyte/logs.py:384-402
Timestamp: 2025-07-11T19:53:44.427Z
Learning: In the PyAirbyte project, when reviewing PRs, maintain clear separation of concerns. Don't suggest changes that are outside the scope of the PR's main objective, even if they would improve consistency or fix other issues. This helps with reviewing changes and potential reverts.
📚 Learning: 2024-10-06T23:44:31.534Z
Learnt from: aaronsteers
Repo: airbytehq/PyAirbyte PR: 411
File: airbyte/cli.py:111-160
Timestamp: 2024-10-06T23:44:31.534Z
Learning: In PyAirbyte, error messages in functions like `_resolve_source_job` in `airbyte/cli.py` are designed to decouple the message text from dynamic values, following a structlog-inspired design. Dynamic values are provided via parameters like `input_value`. This approach helps avoid including PII in the message strings, which may be used in telemetry.
Applied to files:
airbyte/mcp/cloud_ops.py
🧬 Code graph analysis (1)
airbyte/mcp/cloud_ops.py (2)
airbyte/exceptions.py (3)
AirbyteError(432-447)AirbyteMissingResourceError(505-509)PyAirbyteInputError(201-210)airbyte/cloud/workspaces.py (3)
organization_id(101-106)organization_name(109-114)workspace_url(83-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (1)
airbyte/mcp/cloud_ops.py (1)
22-22: No action needed.AirbyteErroris the correct exception type for 403 permission errors. The_make_config_api_request()function inapi_util.pyexplicitly catches HTTP errors and raisesAirbyteErrorfor any non-2xx response, including 403 Forbidden. The try-except block incloud_ops.pycorrectly handles this exception when accessing organization information without ORGANIZATION_READER permissions.
Aaron ("AJ") Steers (aaronsteers)
changed the title
…hod with overloads - Create CloudOrganization dataclass as a minimal value object - Add get_organization(raise_on_error: bool = True) method to CloudWorkspace - Add overloads for proper type inference based on raise_on_error parameter - Remove organization_id and organization_name properties (breaking change) - Update check_airbyte_cloud_workspace MCP tool to use new method This is a breaking change: callers should use workspace.get_organization() instead of workspace.organization_id/organization_name. Co-Authored-By: AJ Steers <aj@airbyte.io>
devin-ai-integration
bot
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…nization_name - Validate that both organization_id and organization_name are non-null and non-empty - If validation fails, raise AirbyteError (or return None if raise_on_error=False) - Update CloudWorkspaceResult docstrings to note ORGANIZATION_READER permission requirement Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
airbyte/cloud/workspaces.py (1)
116-131: The overload pattern looks correct, but note the CodeQL warnings.The overload declarations are correctly structured to provide proper type hints:
- Default/
raise_on_error=True→CloudOrganization(non-nullable)raise_on_error=False→CloudOrganization | NonePast review flagged that
...triggers "Statement has no effect" warnings from CodeQL. While...(Ellipsis) is the idiomatic Python way to write stub method bodies, you could usepassinstead to silence these warnings if they're noisy in your CI, wdyt?@overload - def get_organization(self) -> CloudOrganization: ... + def get_organization(self) -> CloudOrganization: + pass @overload def get_organization( self, *, raise_on_error: Literal[True], - ) -> CloudOrganization: ... + ) -> CloudOrganization: + pass @overload def get_organization( self, *, raise_on_error: Literal[False], - ) -> CloudOrganization | None: ... + ) -> CloudOrganization | None: + pass
🧹 Nitpick comments (1)
airbyte/mcp/cloud_ops.py (1)
497-502: Consider makingorganization_idnullable instead of using a placeholder string?I notice from the past review discussion that using a placeholder string
"[unavailable - requires ORGANIZATION_READER permission]"fororganization_idwas flagged as potentially problematic for downstream code that might:
- Validate UUID format
- Use the ID in API calls
- Store it in databases with constraints
The current
CloudWorkspaceResult.organization_idis typed asstr(line 161). Would it make sense to change this tostr | Noneand returnNonewhen unavailable, similar to howorganization_nameis handled? This would be more type-safe and prevent potential runtime errors downstream, wdyt?That said, I see this was discussed and the current approach was approved, so feel free to keep as-is if there's context I'm missing!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte/cloud/workspaces.py(5 hunks)airbyte/mcp/cloud_ops.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: yohannj
Repo: airbytehq/PyAirbyte PR: 716
File: airbyte/logs.py:384-402
Timestamp: 2025-07-11T19:53:44.427Z
Learning: In the PyAirbyte project, when reviewing PRs, maintain clear separation of concerns. Don't suggest changes that are outside the scope of the PR's main objective, even if they would improve consistency or fix other issues. This helps with reviewing changes and potential reverts.
📚 Learning: 2025-07-11T19:53:44.427Z
Learnt from: yohannj
Repo: airbytehq/PyAirbyte PR: 716
File: airbyte/logs.py:384-402
Timestamp: 2025-07-11T19:53:44.427Z
Learning: In the PyAirbyte project, when reviewing PRs, maintain clear separation of concerns. Don't suggest changes that are outside the scope of the PR's main objective, even if they would improve consistency or fix other issues. This helps with reviewing changes and potential reverts.
Applied to files:
airbyte/mcp/cloud_ops.py
🧬 Code graph analysis (2)
airbyte/mcp/cloud_ops.py (1)
airbyte/cloud/workspaces.py (5)
get_organization(117-117)get_organization(120-124)get_organization(127-131)get_organization(133-180)workspace_url(98-100)
airbyte/cloud/workspaces.py (1)
airbyte/exceptions.py (1)
AirbyteError(432-447)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (4)
airbyte/mcp/cloud_ops.py (1)
161-164: LGTM!The updated docstrings clearly communicate the permission requirements for these fields. This helps consumers of the API understand why these values might be unavailable. Nice touch! 👍
airbyte/cloud/workspaces.py (3)
41-41: LGTM!The new imports for
overloadandAirbyteErrorare correctly added to support the newget_organizationmethod.Also applies to: 55-55
65-77: LGTM!The
CloudOrganizationdataclass is clean and minimal - exactly what was requested. Nice use of a value object to encapsulate the organization info!
133-180: LGTM! Clean implementation with proper error handling.The implementation looks solid:
- Correctly catches
AirbyteErrorfor API/permission failures (confirmed in past discussion that this is the right exception for 403 errors)- Validates that both
organization_idandorganization_nameare non-null and non-empty, as requested- The conditional re-raise pattern (
if raise_on_error: raise) is clean- Good use of context in the error message for debugging
Aaron ("AJ") Steers (aaronsteers)
marked this pull request as ready for review
Aaron ("AJ") Steers (aaronsteers)
changed the title
Aaron ("AJ") Steers (aaronsteers)
changed the title
Aaron ("AJ") Steers (aaronsteers)
merged commit a7c9ccf
into
main
Aaron ("AJ") Steers (aaronsteers)
deleted the
devin/1765397982-fix-check-workspace-permissions
branch
1 participant
Summary
Fixes a regression introduced in v0.33.7 where
check_airbyte_cloud_workspacefails with a 403 error for users with workspace-scoped credentials that lack ORGANIZATION_READER permissions.Approach: Per AJ's request, this refactors the organization info handling upstream to
CloudWorkspacerather than just catching exceptions in the MCP tool:CloudOrganizationdataclass - Minimal value object withorganization_idandorganization_nameget_organization(raise_on_error: bool = True)method - With overloads for proper type inference:get_organization()→CloudOrganization(non-nullable)get_organization(raise_on_error=False)→CloudOrganization | Noneorganization_idandorganization_nameproperties -organization_idandorganization_namemust be non-null and non-empty; otherwise treated as an errorUsers affected by the original issue can pin to
v0.33.6as a workaround.Review & Testing Checklist for Human
CloudWorkspace.organization_idorCloudWorkspace.organization_namepropertiesAirbyteErroris raised for 403 errors (verified atairbyte/_util/api_util.pyline ~1206, but worth double-checking)organization_idANDorganization_nameto be non-empty is the desired behaviorRecommended test plan:
check_airbyte_cloud_workspaceand verify it returns successfully withorganization_id = "[unavailable - requires ORGANIZATION_READER permission]"Notes
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.